[CTL] Add support for defaults #1320

KFilipek · 2025-05-19T09:01:21Z

Description

Checklist

Code compiles without errors locally
All tests pass locally
CI workflows execute properly

src/pool/pool_disjoint.c

src/memory_pool.c

include/umf/memory_pool_ops.h

lukaszstolarczuk · 2025-05-19T10:59:47Z

test/ctl/ctl_api.cpp

+        umfDisjointPoolParamsDestroy(params);
+    } catch (Pool::CtlException &e) {
+        umfDisjointPoolParamsDestroy(params);
+        GTEST_SKIP() << e.what();


why skip, can't we fail?

This approach is commonly used in our tests:

ctest --verbose | grep SKIP | wc -l 295

in some cases SKIP is acceptable, here I believe occurence of CtlException may be caused by a real issue

I removed CtlException completely

lukaszstolarczuk · 2025-05-19T11:05:54Z

test/pools/disjoint_pool.cpp

+    umf_memory_provider_handle_t provider_handle = nullptr;
+    umf_memory_pool_handle_t pool = NULL;
+
+    struct memory_provider : public umf_test::provider_base_t {


do we need this struct in this test?

This should be reworked separately. It follows the approach used in the disjoint pool tests.

ok, so pls leave the struct but don't add the dead code which is the body of the struct with redundant functions

Removed boilerplate.

test/pools/disjoint_pool.cpp

lukaszstolarczuk · 2025-05-19T11:07:02Z

test/pools/disjoint_pool_ctl.cpp

+        auto res = umfPoolCreate(m_poolOps, m_provider, m_params, 0, &m_pool);
+        if (res != UMF_RESULT_SUCCESS) {
+            m_pool = nullptr;
+            LOG_ERR("Failed to create memory pool");


not sure if LOG in test is expected

Done.
Removed.

FYI, if you still want to print some error msg in tests you can just use printf or something..

I believe unit tests should not print logs.

test/pools/disjoint_pool_ctl.cpp

lplewa · 2025-05-19T11:07:49Z

src/libumf.c

+
+umf_result_t umfCtlGet(const char *name, void *ctx, void *arg, size_t size) {
+    if (name == NULL || arg == NULL) {
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }
+    return ctl_query(NULL, ctx, CTL_QUERY_PROGRAMMATIC, name, CTL_QUERY_READ,
+                     arg, size);
+}
+
+umf_result_t umfCtlSet(const char *name, void *ctx, void *arg, size_t size) {
+    // Context can be NULL when setting defaults
+    if (name == NULL || arg == NULL || size == 0) {
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }
+
+    return ctl_query(NULL, ctx, CTL_QUERY_PROGRAMMATIC, name, CTL_QUERY_WRITE,
+                     arg, size)
+               ? UMF_RESULT_ERROR_UNKNOWN
+               : UMF_RESULT_SUCCESS;
+}
+
+umf_result_t umfCtlExec(const char *name, void *ctx, void *arg, size_t size) {
+    if (name == NULL || arg == NULL || ctx == NULL) {
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }
+    return ctl_query(NULL, ctx, CTL_QUERY_PROGRAMMATIC, name,
+                     CTL_QUERY_RUNNABLE, arg, size)
+               ? UMF_RESULT_ERROR_UNKNOWN
+               : UMF_RESULT_SUCCESS;
+}


there is chaos in this inval check - please add comment per each function why we not check for null.

For sure exec should accept null arg.

lplewa · 2025-05-19T11:08:48Z

src/memory_pool.c

+    if (ctx && strstr(extra_name, "default")) {
+        utils_mutex_unlock(&ctl_mtx);
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }


Do we need this strstr? If yes what we are looking for?

lplewa · 2025-05-19T11:11:16Z

src/memory_provider.c

@@ -22,13 +22,13 @@
 #include "utils_assert.h"



Where is support for defaults for memory providers? I see only implementation for pools, and we need both

Should be splitted between 2 PR's

lplewa · 2025-05-19T11:14:13Z

include/umf/memory_pool_ops.h

+    /// @brief Retrieves the name of the disjoint memory pool [optional]
+    /// @param pool pointer to the memory pool or NULL value
+    /// @return A constant character string representing the pool's name.
+    ///         Returns "disjoint" if `pool` is NULL, otherwise returns the
+    ///         configured name of the specific pool instance.
+    ///


Please document requirements of interface - not how this function behaviors for disjoint pool

lplewa · 2025-05-19T11:23:12Z

src/ctl/ctl.h

+/*
+ * Declaration of a new RW leaf. This type of RW leaf doesn't require parsing
+ * of the argument. The argument is passed directly to the read/write callback.
+ */
+#define CTL_LEAF_RW_no_arg(name, ...)                                          \
+    {                                                                          \
+        CTL_STR(name), CTL_NODE_LEAF,                                          \
+            {CTL_READ_HANDLER(name, __VA_ARGS__),                              \
+             CTL_WRITE_HANDLER(name, __VA_ARGS__), NULL, NULL},                \
+            NULL, NULL                                                         \
+    }
+


Why we need this?

As discussed, done.

lukaszstolarczuk · 2025-05-19T11:28:44Z

test/ctl/ctl_unittest.cpp

@@ -21,35 +21,35 @@ TEST_F(test, ctl_debug_read_from_string) {

    int value = 0;
    ctl_query(ctl_handler, NULL, CTL_QUERY_PROGRAMMATIC,
-              "debug.heap.alloc_pattern", CTL_QUERY_READ, &value);
+              "debug.heap.alloc_pattern", CTL_QUERY_READ, &value, 0);


my comment from the prev. PR is not resolved:

whenever size is used, please add a test with various values (user may give some input we may not expect, e.g. on wrong assumptions on how this func works) or mark a TODO for adding tests in the future

e.g. here you can simply add a repeated queries with non-zero values

src/ctl/ctl.c

src/memory_pool.c

src/pool/pool_disjoint.c

test/pools/disjoint_pool.cpp

lukaszstolarczuk · 2025-05-23T10:03:55Z

include/umf/memory_pool.h

+///
+/// @brief Retrieve name of a given memory \p pool.
+/// @param pool handle to the memory pool
+/// @return pointer to a string containing the name of the \p pool


, or NULL when ...

lukaszstolarczuk · 2025-05-23T10:06:03Z

include/umf/memory_pool_ops.h

    /// @param operationType type of the operation to be performed.
    /// @param name name associated with the operation.
    /// @param arg argument for the operation.
+    /// @param size size of the argument [optional - check path requirements]


I meant english word "context" not a parameter called context; nontheless, IMHO, this is still ambiguous - I'm really not sure what/where should I check

lukaszstolarczuk · 2025-05-23T10:07:05Z

include/umf/memory_provider_ops.h

@@ -259,12 +259,13 @@ typedef struct umf_memory_provider_ops_t {
    /// @param operationType type of the operation to be performed.
    /// @param name name associated with the operation.
    /// @param arg argument for the operation.
+    /// @param size size of the argument [optional - check path requirements]
    /// @param queryType type of the query to be performed.
    ///
    /// @return umf_result_t result of the control operation.
    ///
    umf_result_t (*ctl)(void *hProvider, int operationType, const char *name,


just a few lines above in the review we can see the change hPool -> pool; similarly here hProvider -> provider

// it's just a nit

lukaszstolarczuk · 2025-05-23T10:15:03Z

test/ctl/ctl_api.cpp

+        umfDisjointPoolParamsDestroy(params);
+    } catch (Pool::CtlException &e) {
+        umfDisjointPoolParamsDestroy(params);
+        GTEST_SKIP() << e.what();


in some cases SKIP is acceptable, here I believe occurence of CtlException may be caused by a real issue

lukaszstolarczuk · 2025-05-23T10:17:38Z

test/pools/disjoint_pool.cpp

+    umf_memory_provider_handle_t provider_handle = nullptr;
+    umf_memory_pool_handle_t pool = NULL;
+
+    struct memory_provider : public umf_test::provider_base_t {


ok, so pls leave the struct but don't add the dead code which is the body of the struct with redundant functions

src/memory_pool.c

lukaszstolarczuk · 2025-05-23T10:30:58Z

test/ctl/ctl_api.cpp

+    // umfCtlGet - invalid arg
+    res = umfCtlGet(valid_path, valid_ctx, NULL, strlen(valid_arg));
+    ASSERT_EQ(res, UMF_RESULT_ERROR_INVALID_ARGUMENT);
+}


can we have invalid ctx as well? (not sure if this makes sense or not)

test/ctl/ctl_api.cpp

lukaszstolarczuk · 2025-05-23T10:32:21Z

test/pools/disjoint_pool_ctl.cpp

+        auto res = umfPoolCreate(m_poolOps, m_provider, m_params, 0, &m_pool);
+        if (res != UMF_RESULT_SUCCESS) {
+            m_pool = nullptr;
+            LOG_ERR("Failed to create memory pool");


FYI, if you still want to print some error msg in tests you can just use printf or something..

bratpiorka · 2025-05-23T12:32:39Z

src/libumf.c

+
+umf_result_t umfCtlExec(const char *name, void *ctx, void *arg, size_t size) {
+    // arg can be NULL when executing a command
+    // ctx can be NULL when executing defaults


ctx can be NULL but you return UMF_RESULT_ERROR_INVALID_ARGUMENT?
additionally, it looks like we don't have a test for this case

Right, fixed

bratpiorka · 2025-05-23T12:35:09Z

src/memory_pool.c

+                                        const char *extra_name,
+                                        umf_ctl_query_type_t queryType) {
+    (void)indexes, (void)source;
+    if (ctx) {


where do you use ctx?

I removed it because the exec can also go through this path

Add function to get pool name

136dc39

KFilipek self-assigned this May 19, 2025

KFilipek requested a review from a team as a code owner May 19, 2025 09:01

KFilipek added the enhancement New feature or request label May 19, 2025

lukaszstolarczuk mentioned this pull request May 19, 2025

[CTL] Add support for defaults #1216

Closed

3 tasks

bratpiorka requested review from lplewa and bratpiorka May 19, 2025 09:28

lukaszstolarczuk reviewed May 19, 2025

View reviewed changes

lplewa requested changes May 19, 2025

View reviewed changes

lukaszstolarczuk reviewed May 19, 2025

View reviewed changes